-
-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inject CSS styling into RMD parametrization app #866
base: main
Are you sure you want to change the base?
Conversation
@@ -203,7 +259,11 @@ knit_params_ask <- function(file = NULL, | |||
params = NULL, | |||
shiny_args = NULL, | |||
save_caption = "Save", | |||
encoding = getOption("encoding")) { | |||
encoding = getOption("encoding"), | |||
html_head_style = c(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option that won't pollute the argument list so much is to have a single argument that accepts a named list.
html_head = list()
Then, if you want to specify some parts:
html_head = list(style = "body { font-size: 15px; }")
Not sure which would be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, when I replied in the email, it put the comments in the wrong place.
I initially had the same idea, but as per @trestletech , the long form is preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could indeed go either way. I agree with @trestletech that R tends to stylistically prefer long argument lists. Even though this isn't normally good form in more well decomposed systems in R where many operations tend to be top level "one button" ones it makes some sense (also forces documentation of the parameters in .Rd as a side benefit).
R/params.R
Outdated
@@ -186,6 +186,56 @@ params_namedList <- function() { | |||
empty | |||
} | |||
|
|||
|
|||
setup_html_head <- function(html_head_style = c(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: all of our other param helper functions have "params" in their name. It's not the best namespacing, but it's something.
R/params.R
Outdated
@@ -194,6 +244,12 @@ params_namedList <- function() { | |||
#' @param shiny_args Additional arguments to \code{\link[shiny:runApp]{runApp}}. | |||
#' @param save_caption Caption to use use for button that saves/confirms parameters. | |||
#' @param encoding The encoding of the input file; see \code{\link{file}}. | |||
#' @param html_head_style a string or a list/vector of strings representing CSS style that will be injected in the HTML HEAD. | |||
#' @param html_head_script a string or a list/vector of strings represeting JS scripts that will be injected in the HTML HEAD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spelling of representing
html_head_style <- as.vector(html_head_style) | ||
custom_styles <- lapply(html_head_style, shiny::tags$style) | ||
|
||
html_head_script <- as.vector(html_head_script) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the as.vector
conversion? Am I missing some subtlety?
a <- c(1,2)
lapply(a, function(b) b*2)
lapply(as.vector(a), function(b) b*2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, sorry, when I replied in the email, it put the comments in the wrong place.
the params can be strings or vector of strings.
"body { padding-bottom: 70px; }" | ||
) | ||
## Escape is "cancel" and Enter is "save". | ||
default_script <- shiny::tags$script(shiny::HTML("$(document).keyup(function(e) {\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any situation where we will want to not inject the defaults?
As per Jeff, the long form is preferred
|
It can be a string or a list of strings
|
Ok tweaked as requested and added tests. This is re-ready for review @trestletech @aronatkins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we work through the Connect integration before calling this final? In particular, I'm interested in how we are going to hide the ok/cancel buttons, suppress the enter/escape handling, and inject ok/cancel actions from the parent iframe.
That's fine. We are going to ship a new version of rmarkdown CRAN around On Mon, Nov 14, 2016 at 2:24 PM, Aron Atkins notifications@github.com
|
This code is designed to override the defaults with the custom. So for What is the use case to insert actions? In any case, all you do is insert Update: after taking a better look at the code, I noticed that there might be enough logic in the shiny R code for inputs, to keep this strategy from working for certain use cases, for example, default (blank) param support. @aronatkins we should touch base in the am so i can get a better idea of what the use cases are |
@@ -250,6 +250,7 @@ params_html_head <- function(html_head_style = c(), | |||
#' You must take care to unsure that the URL is absolute. | |||
#' @param html_head_script_link same as above except that these are interpreted as SRC attributes in SCRIPT tags. | |||
#' You must take care to unsure that the URL is absolute. | |||
#' @param disable_bootstrap if true, does not inject boostrap scripts and styles in the HTML HEAD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the common formatting is for this package, but you could consider styling with if \code{TRUE}, does not...
Probably more important to align with the conventions of the package but just making sure you're aware of the option.
9ce8b28
to
2e8b846
Compare
Uriel Avalos seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Modified
knit_params_ask
so that it is possible to pass custom css/scripts, both as strings and as links to URIs.Connected to #3945